OCPCLOUD-3214: Add ClusterAPI config#2564
OCPCLOUD-3214: Add ClusterAPI config#2564openshift-merge-bot[bot] merged 2 commits intoopenshift:masterfrom
Conversation
|
Pipeline controller notification For optional jobs, comment |
openshift-ci-robot
added
the
jira/valid-reference
|
@mdbooth: This pull request references OCPCLOUD-3214 which is a valid jira issue. Warning: The referenced jira issue has an invalid target version for the target branch this PR targets: expected the story to target the "4.21.0" version, but no target version was set. DetailsIn response to this: Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the openshift-eng/jira-lifecycle-plugin repository. |
|
Hello @mdbooth! Some important instructions when contributing to openshift/api: |
openshift-ci
bot
added
the
size/XXL
openshift-ci
bot
added
the
do-not-merge/work-in-progress
mdbooth
force-pushed
the
ccapio-staged
branch
from
1eb67fc to
5e7a826
Compare
openshift-ci
bot
removed
the
do-not-merge/work-in-progress
| # TODO: Cannot test the XValidation rule "unmanagedCustomResourceDefinitions cannot be unset once set" | ||
| # because MinProperties=1 on ClusterAPISpec prevents creating spec: {} in the first place. | ||
| # The validation is still enforced at the API level via the XValidation rule. |
There was a problem hiding this comment.
Shouldn't this work? (I tried it)
| # TODO: Cannot test the XValidation rule "unmanagedCustomResourceDefinitions cannot be unset once set" | |
| # because MinProperties=1 on ClusterAPISpec prevents creating spec: {} in the first place. | |
| # The validation is still enforced at the API level via the XValidation rule. | |
| - name: Should not allow unsetting unmanagedCustomResourceDefinitions | |
| initial: | | |
| apiVersion: operator.openshift.io/v1alpha1 | |
| kind: ClusterAPI | |
| metadata: | |
| name: cluster | |
| spec: | |
| unmanagedCustomResourceDefinitions: | |
| - clusters.cluster.x-k8s.io | |
| - machines.cluster.x-k8s.io | |
| updated: | | |
| apiVersion: operator.openshift.io/v1alpha1 | |
| kind: ClusterAPI | |
| metadata: | |
| name: cluster | |
| spec: | |
| unmanagedCustomResourceDefinitions: null | |
| expectedError: "unmanagedCustomResourceDefinitions cannot be unset once set" |
There was a problem hiding this comment.
I don't think set to null is the same as unset.
However, I decided to go a different direction with this: I made spec optional. I think this should be the default for a ClusterOperator, tbh.
mdbooth
force-pushed
the
ccapio-staged
branch
2 times, most recently
from
c496532 to
465771a
Compare
yuqi-zhang
left a comment
yuqi-zhang
left a comment
There was a problem hiding this comment.
General structure looks fine, some questions inline
Also, based on Claude suggestion, all optional fields (spec, status, UnmanagedCustomResourceDefinitions, TargetConfigMaps, ActiveConfigMaps) should have a godoc line explaining what happens if the value is empty. (example: When omitted, the capi-operator will use default configuration with all CRDs managed by the operator. in the spec, assuming we keep spec optional)
| // ClusterAPIStatus describes the current state of the capi-operator. | ||
| // +kubebuilder:validation:MinProperties=1 | ||
| type ClusterAPIStatus struct { | ||
| // targetConfigMaps is a list of ConfigMap names that the staging controller |
There was a problem hiding this comment.
I'm probably missing some context but I'm not 100% sure what these two configmap status field entails. I assume these are here for an admin to see? Are these reflecting the status of some other action the admin took elsewhere? Or something that has to do with the CRDs they just defined in the spec? (Just from an ELI5 perspective the status fields are confusing, if there's some other way to add some more context I think that would be helpful)
There was a problem hiding this comment.
I need to re-write this (again), but this is for internal state of the installer controller.
openshift-ci
bot
added
the
do-not-merge/work-in-progress
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughIntroduces a new ClusterAPI v1alpha1 API: types (ClusterAPI, ClusterAPISpec, ClusterAPIStatus, ClusterAPIInstallerRevision, ClusterAPIInstallerComponent, ClusterAPIInstallerComponentImage, ClusterAPIList, RevisionName, ImageDigestFormat), scheme registration, autogenerated deepcopy methods, OpenAPI and Swagger definitions. Adds CRD manifests (DevPreview, TechPreview, CustomNoUpgrade) and a feature-gated CRD manifest, a comprehensive test YAML for ClusterAPI scenarios, updates payload/script globs to include the CRDs, and adds a golangci-lint exclusion. 🚥 Pre-merge checks | ✅ 1 | ❌ 2❌ Failed checks (1 warning, 1 inconclusive)
✅ Passed checks (1 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing touches🧪 Generate unit tests (beta)
Comment |
openshift-ci
bot
removed
the
do-not-merge/work-in-progress
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 Fix all issues with AI Agents
In @openapi/generated_openapi/zz_generated.openapi.go:
- Around line 60773-60792: The generated OpenAPI function
schema_openshift_api_operator_v1alpha1_ClusterAPIInstallerComponentImage is
missing the string validation constraints from
machineosconfigv1.ImageDigestFormat; update the "digest" property's SchemaProps
to include MinLength: 1, MaxLength: 447 and the appropriate Pattern (the regex
that enforces host[:port][/namespace]/name@sha256:<64-lowercase-hex> or at
minimum the @sha256:[a-f0-9]{64} portion used in the kubebuilder XValidation),
so the OpenAPI output matches the source kubebuilder markers and preserves the
same validation semantics.
In @operator/v1alpha1/zz_generated.swagger_doc_generated.go:
- Around line 167-177: Add a type-level doc comment to the
ClusterAPIInstallerRevision type declaration in
operator/v1alpha1/types_clusterapi.go so the swagger generator emits the
empty-string key; specifically, locate the type named
ClusterAPIInstallerRevision and add a brief comment line immediately above it
(e.g., "// ClusterAPIInstallerRevision ...") describing the type, then re-run
hack/update-swagger-docs.sh to regenerate zz_generated.swagger_doc_generated.go
which will include the "" key in map_ClusterAPIInstallerRevision.
🧹 Nitpick comments (2)
operator/v1alpha1/zz_generated.featuregated-crd-manifests.yaml (1)
17-17: Consider adding printer columns for better kubectl output.The PrinterColumns array is empty, which means
kubectl get clusterapiswill only show default columns (NAME, AGE). Adding custom columns for key status fields would improve usability.Example columns to consider:
- Status/Phase
- Observed generation
- Current revision
This can be addressed in a follow-up as the API stabilizes.
operator/v1alpha1/tests/clusterapis.operator.openshift.io/ClusterAPIMachineManagement.yaml (1)
460-551: LGTM!The revision ordering tests correctly enforce that new revisions must have a revision number greater than all existing revisions, aligning with the monotonically increasing requirement.
Consider adding a test case for attempting to delete all revisions (leaving an empty list), which should fail due to the
MinItems=1constraint onrevisions. This would provide complete coverage of the boundary conditions.
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Cache: Disabled due to data retention organization setting
Knowledge base: Disabled due to data retention organization setting
⛔ Files ignored due to path filters (4)
operator/v1alpha1/zz_generated.crd-manifests/0000_30_cluster-api_01_clusterapis-CustomNoUpgrade.crd.yamlis excluded by!**/zz_generated.crd-manifests/*operator/v1alpha1/zz_generated.crd-manifests/0000_30_cluster-api_01_clusterapis-DevPreviewNoUpgrade.crd.yamlis excluded by!**/zz_generated.crd-manifests/*operator/v1alpha1/zz_generated.crd-manifests/0000_30_cluster-api_01_clusterapis-TechPreviewNoUpgrade.crd.yamlis excluded by!**/zz_generated.crd-manifests/*operator/v1alpha1/zz_generated.featuregated-crd-manifests/clusterapis.operator.openshift.io/ClusterAPIMachineManagement.yamlis excluded by!**/zz_generated.featuregated-crd-manifests/**
📒 Files selected for processing (8)
openapi/generated_openapi/zz_generated.openapi.goopenapi/openapi.jsonoperator/v1alpha1/register.gooperator/v1alpha1/tests/clusterapis.operator.openshift.io/ClusterAPIMachineManagement.yamloperator/v1alpha1/types_clusterapi.gooperator/v1alpha1/zz_generated.deepcopy.gooperator/v1alpha1/zz_generated.featuregated-crd-manifests.yamloperator/v1alpha1/zz_generated.swagger_doc_generated.go
🧰 Additional context used
🧬 Code graph analysis (3)
operator/v1alpha1/types_clusterapi.go (1)
operator/v1alpha1/zz_generated.swagger_doc_generated.go (6)
ClusterAPISpec(194-196)ClusterAPIStatus(205-207)ClusterAPIInstallerRevision(175-177)ClusterAPIInstallerComponent(154-156)ClusterAPIInstallerComponentImage(163-165)ClusterAPI(145-147)
operator/v1alpha1/zz_generated.swagger_doc_generated.go (1)
operator/v1alpha1/types_clusterapi.go (7)
ClusterAPI(25-40)ClusterAPIInstallerComponent(154-159)ClusterAPIInstallerComponentImage(162-169)ClusterAPIInstallerRevision(112-150)ClusterAPIList(177-186)ClusterAPISpec(43-65)ClusterAPIStatus(77-110)
operator/v1alpha1/zz_generated.deepcopy.go (1)
operator/v1alpha1/types_clusterapi.go (7)
ClusterAPI(25-40)ClusterAPISpec(43-65)ClusterAPIInstallerComponent(154-159)ClusterAPIInstallerComponentImage(162-169)ClusterAPIInstallerRevision(112-150)ClusterAPIList(177-186)ClusterAPIStatus(77-110)
🔇 Additional comments (30)
operator/v1alpha1/zz_generated.featuregated-crd-manifests.yaml (1)
1-23: Feature gate "ClusterAPIMachineManagement" is properly defined.The CRD manifest correctly references the feature gate, which is properly registered in
features/features.go(line 555) with appropriate scope (DevPreviewNoUpgrade, TechPreviewNoUpgrade) and annotation intypes_clusterapi.go(line 17). The structure and formatting are correct and follow the established pattern.operator/v1alpha1/register.go (1)
44-45: Registration follows Kubernetes conventions. BothClusterAPIandClusterAPIListtypes are properly defined and correctly registered. The types implement standard Kubernetes patterns withTypeMeta,ObjectMeta/ListMeta, and appropriate resource/list fields.openapi/openapi.json (2)
35322-35424: ClusterAPI schema definitions look structurally sound.The ClusterAPI, ClusterAPIList, ClusterAPISpec, and ClusterAPIStatus definitions follow Kubernetes conventions and include comprehensive validation rules. Field descriptions for
unmanagedCustomResourceDefinitions,activeConfigMaps, andtargetConfigMapsare detailed with DNS-1123 requirements and cardinality limits, which is appropriate. The use ofx-kubernetes-list-type: "set"and"atomic"is correct for their respective semantics.Since this appears to be auto-generated from source code, ensure that the Go type definitions in the operator/v1alpha1 package match this schema (required fields, field names, validation tags). Given that only this generated file is provided for review, the source code should be verified separately.
51623-51692: Info type definition: verify namespace and required-field defaults.The
io.k8s.apimachinery.pkg.version.Infotype defines versioning metadata with nine required fields and four optional fields. However, there's a potential inconsistency:
Namespace discrepancy: The AI summary references this as
io.k8s.apimachinery.pkg.apis.meta.v1.Info, but the schema showsio.k8s.apimachinery.pkg.version.Info. Confirm the correct namespace is intended and matches the source code.Required fields with defaults: All nine required fields have
"default": ""(empty string). For fields marked as required, having a default value is semantically unusual—clarify whether these should actually be optional or if the defaults should be removed.Optional field descriptions: Unlike Hunk 1, the descriptions for emulation and compatibility fields lack details about valid formats or constraints. Consider aligning documentation style if consistency is desired.
operator/v1alpha1/tests/clusterapis.operator.openshift.io/ClusterAPIMachineManagement.yaml (5)
1-82: LGTM!The
onCreatetests provide comprehensive coverage for ClusterAPI creation scenarios, including minimal creation, valid/invalidunmanagedCustomResourceDefinitionsentries, and DNS-1123 subdomain validation.
83-150: LGTM!The
onUpdatetests forunmanagedCustomResourceDefinitionscorrectly validate immutability constraints: items cannot be removed, the list cannot be unset once populated, but additions are permitted.
151-283: LGTM!The status subresource tests correctly validate revision uniqueness constraints (unique names, unique revision numbers) and the
desiredRevisionrequirement to reference the highest revision number.
284-378: LGTM!The
currentRevisionvalidation tests properly cover the constraint that it must reference an existing revision entry when set, while correctly allowing it to be absent.
379-459: LGTM!The revision immutability and deletion tests correctly validate that existing revisions cannot be modified but can be deleted. The test at lines 410-458 appropriately verifies that removing old revisions is permitted.
operator/v1alpha1/zz_generated.swagger_doc_generated.go (2)
138-166: LGTM!The SwaggerDoc entries for
ClusterAPI,ClusterAPIInstallerComponent, andClusterAPIInstallerComponentImageare consistent with their type definitions and follow the established pattern in this file.
179-207: LGTM!The SwaggerDoc entries for
ClusterAPIList,ClusterAPISpec, andClusterAPIStatusare well-documented and align with the field-level comments in the source type definitions.openapi/generated_openapi/zz_generated.openapi.go (6)
1194-1200: LGTM!The schema registrations are correctly formatted and follow the established naming conventions for the ClusterAPI v1alpha1 types.
60700-60748: LGTM!The ClusterAPI schema definition follows standard Kubernetes resource conventions with proper metadata, spec, and status fields.
60750-60771: LGTM!The ClusterAPIInstallerComponent schema correctly defines the required image field with proper type reference.
60869-60918: LGTM!The ClusterAPIList schema follows standard Kubernetes list resource conventions with proper metadata and items fields.
60920-60951: The source field already includes all required validation markers.The
UnmanagedCustomResourceDefinitionsfield inoperator/v1alpha1/types_clusterapi.go(lines 59-64) contains all four validation markers:
+kubebuilder:validation:MaxItems=128(line 60)+kubebuilder:validation:items:MaxLength=253(line 63)+kubebuilder:validation:items:XValidationfor DNS-1123 subdomain format (line 62)+kubebuilder:validation:XValidationfor immutability (line 61)These validations are not visible in the generated OpenAPI Go code because generated OpenAPI definitions don't serialize kubebuilder validation markers—they are processed separately by controller-gen to produce CRD manifests and validation rules.
Likely an incorrect or invalid review comment.
60953-61000: All validation constraints are properly enforced in the source type definition. Therevisionsfield inoperator/v1alpha1/types_clusterapi.goincludes:
+kubebuilder:validation:MaxItems=16+kubebuilder:validation:XValidationfor unique names, unique revision numbers, immutability, and revision number orderingThe generated OpenAPI schema does not include CEL validation rules because the OpenAPI 3.0 specification has limited support for such constraints—this is expected and correct behavior. The constraints are enforced at the API type level, which is what matters for validation.
Likely an incorrect or invalid review comment.
operator/v1alpha1/types_clusterapi.go (7)
1-7: LGTM!Package declaration and imports are appropriate for the ClusterAPI type definitions.
9-40: Well-structured ClusterAPI resource definition.The struct correctly implements singleton enforcement (line 24), feature gating (line 17), and proper immutability constraints (line 23) for the unmanagedCustomResourceDefinitions field. The use of a pointer for Spec and
omitzerofor Status follows the conventions discussed in prior reviews.
42-65: Excellent validation rules for the spec.The validation annotations correctly enforce:
- No duplicate CRD names (
listType=setat line 59)- Append-only semantics (line 61)
- Proper DNS-1123 subdomain format (line 62)
- Reasonable size limits (lines 60, 63)
The validation logic is sound and aligns with the documented use case.
67-110: Sophisticated and correct validation logic for status management.The CEL validation rules are well-crafted:
- Line 75 correctly ensures
desiredRevisionreferences the revision with the highest number- Line 76 ensures
currentRevisionconsistency with the revisions list- Lines 105-108 enforce critical invariants: unique names, unique numbers, monotonic increases, and immutability
These validations effectively prevent invalid state transitions in the revision management system.
112-150: Well-defined revision structure.The ClusterAPIInstallerRevision type properly defines:
- Required identification fields (Name, Revision, ContentID)
- Consistent validation for UnmanagedCustomResourceDefinitions matching the spec
- Ordered component list with
listType=atomic(line 146) - appropriate for installation orderingThe field constraints and validations are appropriate for the revision management use case.
152-169: Good extensibility design for component sources.The
MaxProperties=1constraint (line 153) on ClusterAPIInstallerComponent creates a union-like structure, enabling future addition of alternative component sources beyond images. The reuse ofmachineosconfigv1.ImageDigestFormatensures consistent validation across the OpenShift API.
171-186: LGTM!Standard Kubernetes list type implementation following established conventions.
operator/v1alpha1/zz_generated.deepcopy.go (6)
29-59: LGTM!The generated DeepCopy implementation for ClusterAPI correctly handles the pointer Spec field (lines 34-38) with proper nil checking and allocation, and properly deep copies ObjectMeta and Status.
61-92: LGTM!The deepcopy implementations for ClusterAPIInstallerComponent and ClusterAPIInstallerComponentImage correctly use simple assignment since these structs contain only value types (no pointers or slices requiring deep copies).
94-118: LGTM!The DeepCopyInto implementation for ClusterAPIInstallerRevision correctly handles both slice fields:
- UnmanagedCustomResourceDefinitions ([]string) with proper allocation and copy
- Components ([]ClusterAPIInstallerComponent) with proper allocation and copy
Nil checks are appropriately placed.
120-151: LGTM!The ClusterAPIList deepcopy implementation correctly iterates through Items and calls DeepCopyInto on each ClusterAPI element (lines 128-130), ensuring proper deep copying of the list contents.
153-172: LGTM!The ClusterAPISpec deepcopy correctly handles the UnmanagedCustomResourceDefinitions string slice with proper nil checking and allocation.
174-195: LGTM!The ClusterAPIStatus deepcopy correctly handles:
- Scalar fields (CurrentRevision, DesiredRevision) with simple assignment
- Revisions slice with proper iteration and nested DeepCopyInto calls (lines 180-182)
The implementation is correct and complete.
| var map_ClusterAPIInstallerRevision = map[string]string{ | ||
| "name": "name is the name of a revision.", | ||
| "revision": "revision is a monotonically increasing number that is assigned to a revision.", | ||
| "contentID": "contentID uniquely identifies the content of this revision. The contentID must be between 1 and 255 characters long.", | ||
| "unmanagedCustomResourceDefinitions": "unmanagedCustomResourceDefinitions is a list of the names of ClusterResourceDefinition (CRD) objects which are included in this revision, but which should not be installed or updated. If not set, all CRDs in the revision will be managed by the CAPI operator.", | ||
| "components": "components is list of components which will be installed by this revision. Components will be installed in the order they are listed.\n\nThe maximum number of components is 32.", | ||
| } | ||
|
|
||
| func (ClusterAPIInstallerRevision) SwaggerDoc() map[string]string { | ||
| return map_ClusterAPIInstallerRevision | ||
| } |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Check if ClusterAPIInstallerRevision has a type-level doc comment in the source
rg -B5 'type ClusterAPIInstallerRevision struct' operator/v1alpha1/types_clusterapi.goRepository: openshift/api
Length of output: 583
🏁 Script executed:
#!/bin/bash
# Get more context before the type declaration
rg -B15 'type ClusterAPIInstallerRevision struct' operator/v1alpha1/types_clusterapi.go | head -25Repository: openshift/api
Length of output: 1233
🏁 Script executed:
#!/bin/bash
# Check if other types have doc comments
rg -B3 'type ClusterAPI struct' operator/v1alpha1/types_clusterapi.go | head -10Repository: openshift/api
Length of output: 514
🏁 Script executed:
#!/bin/bash
# Check the ClusterAPIInstallerComponent type
rg -B3 'type ClusterAPIInstallerComponent struct' operator/v1alpha1/types_clusterapi.go | head -10Repository: openshift/api
Length of output: 237
🏁 Script executed:
#!/bin/bash
# Check the generated swagger doc file to see pattern of empty string keys
rg -A8 'var map_ClusterAPI = map\[string\]string' operator/v1alpha1/zz_generated.swagger_doc_generated.goRepository: openshift/api
Length of output: 756
🏁 Script executed:
#!/bin/bash
# Check ClusterAPIInstallerComponent map
rg -A8 'var map_ClusterAPIInstallerComponent = map\[string\]string' operator/v1alpha1/zz_generated.swagger_doc_generated.goRepository: openshift/api
Length of output: 490
🏁 Script executed:
#!/bin/bash
# Check ClusterAPIList map
rg -A8 'var map_ClusterAPIList = map\[string\]string' operator/v1alpha1/zz_generated.swagger_doc_generated.goRepository: openshift/api
Length of output: 651
🏁 Script executed:
#!/bin/bash
# Get the full ClusterAPIInstallerRevision type with all context
rg -A20 'type ClusterAPIInstallerRevision struct {' operator/v1alpha1/types_clusterapi.goRepository: openshift/api
Length of output: 202
🏁 Script executed:
#!/bin/bash
# Get the full ClusterAPIInstallerRevision type with all context (escaping braces)
rg -A20 'type ClusterAPIInstallerRevision struct' operator/v1alpha1/types_clusterapi.goRepository: openshift/api
Length of output: 915
Add type-level documentation comment for ClusterAPIInstallerRevision.
The map_ClusterAPIInstallerRevision in the generated swagger doc lacks an empty string key for type-level documentation, unlike all other type maps in this file (map_ClusterAPI, map_ClusterAPIInstallerComponent, map_ClusterAPIList, etc.). This inconsistency exists because the ClusterAPIInstallerRevision type in types_clusterapi.go is missing a doc comment.
Add a doc comment to the ClusterAPIInstallerRevision type definition in operator/v1alpha1/types_clusterapi.go. The auto-generated swagger doc file will be regenerated via hack/update-swagger-docs.sh to include the empty string key.
🤖 Prompt for AI Agents
In @operator/v1alpha1/zz_generated.swagger_doc_generated.go around lines 167 -
177, Add a type-level doc comment to the ClusterAPIInstallerRevision type
declaration in operator/v1alpha1/types_clusterapi.go so the swagger generator
emits the empty-string key; specifically, locate the type named
ClusterAPIInstallerRevision and add a brief comment line immediately above it
(e.g., "// ClusterAPIInstallerRevision ...") describing the type, then re-run
hack/update-swagger-docs.sh to regenerate zz_generated.swagger_doc_generated.go
which will include the "" key in map_ClusterAPIInstallerRevision.
8f7329e to
04dd50a
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In `@openapi/openapi.json`:
- Around line 35379-35481: The OpenAPI schema is out of sync with the Go types:
remove the spurious properties "activeConfigMaps" and "targetConfigMaps" from
the definition "com.github.openshift.api.operator.v1alpha1.ClusterAPIStatus"
(these fields do not exist in operator/v1alpha1/types_clusterapi.go), and add
the missing maxItems: 128 constraint to "unmanagedCustomResourceDefinitions" in
"com.github.openshift.api.operator.v1alpha1.ClusterAPISpec" (it currently only
has a descriptive note). After making those changes, regenerate the OpenAPI
schema from the Go types/CRD so the JSON matches the real API definitions.
♻️ Duplicate comments (3)
operator/v1alpha1/zz_generated.swagger_doc_generated.go (1)
168-173: Add type-level doc comment forClusterAPIInstallerRevision.The generated map is missing the top-level
""entry, which usually means the type inoperator/v1alpha1/types_clusterapi.golacks a doc comment. Add a short type comment there and regenerate docs.openapi/generated_openapi/zz_generated.openapi.go (2)
60867-60893: Missing validation constraints in generated schema (previously flagged).The
refandprofilefields are missing string validation constraints (minLength,maxLength,pattern) that should be generated from kubebuilder markers in the source type. This was raised in a previous review and requires fixing the source type annotations or the code generation pipeline.
60895-60968: Missing validation constraints in generated schema (previously flagged).The
contentIDfield is missingminLength/maxLengthconstraints, and thecomponentsarray is missingmaxItemsvalidation. This was raised in a previous review and requires fixing the source type annotations or the code generation pipeline.
mdbooth
force-pushed
the
ccapio-staged
branch
from
04dd50a to
e01cd64
Compare
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Fix all issues with AI agents
In `@openapi/generated_openapi/zz_generated.openapi.go`:
- Around line 61028-61047: The generated OpenAPI for the
unmanagedCustomResourceDefinitions field is missing MaxItems=128 and item
MaxLength=253 from the source kubebuilder markers; update the OpenAPI generation
mapping so the kubebuilder validation tags on the source type
(operator/v1alpha1/types_clusterapi.go) are propagated into the generated schema
for the "unmanagedCustomResourceDefinitions" field: set SchemaProps.MaxItems =
128 on the array schema and SchemaProps.MaxLength = 253 on the item string
schema (the nested Schema under Items) during generation (i.e., in the code that
translates kubebuilder validation markers to spec.SchemaProps so the
zz_generated.openapi.go includes MaxItems and MaxLength).
- Around line 61075-61093: The generated OpenAPI schema for the "revisions"
array is missing the MaxItems constraint declared in the source
(+kubebuilder:validation:MaxItems=16); update the SchemaProps for "revisions" to
include MaxItems: 16 (as the appropriate integer type expected by
spec.SchemaProps) so the array validation matches the marker. Locate the
"revisions" SchemaProps block in the generated_openapi (the block that sets
Type: []string{"array"} and Items: &spec.SchemaOrArray{...}) and add MaxItems:
16, or fix the generator/regeneration step so MaxItems is emitted automatically.
♻️ Duplicate comments (3)
openapi/openapi.json (1)
35443-35455: OpenAPI schema looks out of sync with ClusterAPI Go types (maxItems + status fields).This appears to repeat the earlier mismatch:
unmanagedCustomResourceDefinitionsstill lacks amaxItems: 128constraint, andactiveConfigMaps/targetConfigMapsare present even if they’re not in the Go type. Please regenerate the OpenAPI from the Go types/CRD and ensure these match.🔧 Minimal fix sketch (prefer regeneration over manual edits)
@@ "unmanagedCustomResourceDefinitions": { "description": "...", "type": "array", "items": { "type": "string", "default": "" }, - "x-kubernetes-list-type": "set" + "x-kubernetes-list-type": "set", + "maxItems": 128 } @@ - "activeConfigMaps": { ... }, - "targetConfigMaps": { ... } + // remove if not present in the Go type definitionsAlso applies to: 35458-35478
operator/v1alpha1/zz_generated.swagger_doc_generated.go (1)
168-178: Missing type-level documentation forClusterAPIInstallerRevision.The
map_ClusterAPIInstallerRevisionlacks an empty string key ("") for the type-level description, unlike all other type maps in this file. This indicates the source type intypes_clusterapi.gois missing a doc comment above the type declaration.Add a doc comment to
ClusterAPIInstallerRevisioninoperator/v1alpha1/types_clusterapi.go, then regenerate this file viahack/update-swagger-docs.sh.openapi/generated_openapi/zz_generated.openapi.go (1)
60867-60893: OpenAPI schema missing validation constraints forrefandprofilefields.The descriptions document validation requirements that aren't reflected in the SchemaProps:
ref(line 60874-60880): Description specifies "1 to 447 characters" and digest format, but SchemaProps lacksMinLength,MaxLength, andPattern.profile(line 60881-60887): Description specifies "up to 255 characters" with alphanumeric/dash/underscore restriction, but SchemaProps lacksMaxLengthandPattern.Since this is generated code, ensure the source type (
operator/v1alpha1/types_clusterapi.go) has the appropriate kubebuilder validation markers that propagate to the OpenAPI output.
| "revisions": { | ||
| VendorExtensible: spec.VendorExtensible{ | ||
| Extensions: spec.Extensions{ | ||
| "x-kubernetes-list-type": "atomic", | ||
| }, | ||
| }, | ||
| SchemaProps: spec.SchemaProps{ | ||
| Description: "revisions is a list of all currently active revisions. A revision is active until the installer controller updates currentRevision to a later revision. It is written by the revision controller.\n\nThe maximum number of revisions is 16. All revisions must have a unique name. All revisions must have a unique revision number. When adding a revision, the revision number must be greater than the highest revision number in the list. Revisions are immutable, although they can be deleted.", | ||
| Type: []string{"array"}, | ||
| Items: &spec.SchemaOrArray{ | ||
| Schema: &spec.Schema{ | ||
| SchemaProps: spec.SchemaProps{ | ||
| Default: map[string]interface{}{}, | ||
| Ref: ref("github.com/openshift/api/operator/v1alpha1.ClusterAPIInstallerRevision"), | ||
| }, | ||
| }, | ||
| }, | ||
| }, | ||
| }, |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
# First, locate the source file with ClusterAPIStatus type definition
find . -name "types_clusterapi.go" -o -name "*clusterapi*types*.go" | head -20Repository: openshift/api
Length of output: 170
🏁 Script executed:
# Search for ClusterAPIStatus definition with Revisions field
rg -B5 -A15 'type ClusterAPIStatus struct' --type goRepository: openshift/api
Length of output: 2249
🏁 Script executed:
# Also check if there's a MaxItems marker in the type definition
rg 'Revisions.*\[\].*MaxItems' --type goRepository: openshift/api
Length of output: 39
🏁 Script executed:
# Get the full Revisions field definition with all annotations
rg -B2 -A2 'Revisions.*\[\]' operator/v1alpha1/types_clusterapi.goRepository: openshift/api
Length of output: 541
🏁 Script executed:
# Search more broadly for revisions field in the ClusterAPIStatus struct
rg -A30 'type ClusterAPIStatus struct' operator/v1alpha1/types_clusterapi.go | head -50Repository: openshift/api
Length of output: 1919
Missing MaxItems constraint on revisions field in generated OpenAPI.
The source type defines +kubebuilder:validation:MaxItems=16 but the generated OpenAPI schema lacks this constraint. The SchemaProps should include MaxItems: 16 to match the validation marker and enforce the documented limitation.
🤖 Prompt for AI Agents
In `@openapi/generated_openapi/zz_generated.openapi.go` around lines 61075 -
61093, The generated OpenAPI schema for the "revisions" array is missing the
MaxItems constraint declared in the source
(+kubebuilder:validation:MaxItems=16); update the SchemaProps for "revisions" to
include MaxItems: 16 (as the appropriate integer type expected by
spec.SchemaProps) so the array validation matches the marker. Locate the
"revisions" SchemaProps block in the generated_openapi (the block that sets
Type: []string{"array"} and Items: &spec.SchemaOrArray{...}) and add MaxItems:
16, or fix the generator/regeneration step so MaxItems is emitted automatically.
|
|
||
| // ClusterAPIStatus describes the current state of the capi-operator. | ||
| // +kubebuilder:validation:MinProperties=1 | ||
| // +kubebuilder:validation:XValidation:rule="self.revisions.exists(r, r.name == self.desiredRevision && self.revisions.all(s, s.revision <= r.revision))",message="desiredRevision must be the name of the revision with the highest revision number" |
There was a problem hiding this comment.
Is it the same controller setting Current/Desired and the Revisions list? I'm curious as to the validations here, since I assume any errors would be handled by the controller as well, whether those can actually drift?
There was a problem hiding this comment.
These are written by different controllers (the revision and installer controllers). The desiredRevision is technically redundant. I don't think the controller actually looks at it, except to compare it to currentRevision. All it's actually doing is installing the most recent revision.
If this validation failed, this would be a bug in one of those controllers. The only real purpose is to throw an error at the point that the bug occurs rather than later. If some other actor wrote a weird value there, the controllers should recover from it.
| // Revisions are immutable, although they can be deleted. | ||
| // | ||
| // +required | ||
| // +listType=atomic |
There was a problem hiding this comment.
Since name (and revision number) are unique, we can probably turn this into a listType=map and use either as the key, and then we can remove the unique validation on that.
There was a problem hiding this comment.
I considered listType=map here, but my expectation is that this list will only ever be written atomically by the revision controller. There's no reason it couldn't be listType=map, but I figured that atomic is a bit cheaper server-side, so we might as well leave it atomic as we don't expect to manage multiple actors.
We could always revisit this for v1 if it becomes an issue?
| // +kubebuilder:validation:Minimum=1 | ||
| Revision int64 `json:"revision,omitempty"` | ||
|
|
||
| // contentID uniquely identifies the content of this revision. |
There was a problem hiding this comment.
Could you add some more description on what the ID entails? I'm not sure I understand why name, revision, and a third ID are all required.
There was a problem hiding this comment.
This is a tough one. On contentID specifically, I don't think so, no 😅
The reason is that this description genuinely does completely define the API contract: that if any content of the revision changes, i.e. what the installer would actually install, then this value will change.
I could write here that it's implemented as a hash of X, Y, and Z, but:
- Nothing ever checks that
- The API consumer can't rely on that
- I want to be able to change it as necessary without API impact
revision is a boxcutter implementation detail. We just need it.
name is intended to be a human-useful value. Technically we could use contentID everywhere it is used, I just figured that the format of name would make it easier to understand what's going on.
| // The digest must be 64 characters long, and consist only of lowercase hexadecimal characters, a-f and 0-9. | ||
| // The length of the field must be between 1 to 447 characters. | ||
| // +required | ||
| Ref machineosconfigv1.ImageDigestFormat `json:"ref,omitempty"` |
There was a problem hiding this comment.
Update: per API guidance, we try not to share types between different APIs, in case it would need to be updated later.
In this case, the suggestion is to make a duplicate copy of the ImageDigestFormat type in this API, instead of referencing the existing object.
| // image defines an image source for a component. The image must contain a | ||
| // /capi-operator-installer directory containing the component manifests. | ||
| // +required | ||
| Image ClusterAPIInstallerComponentImage `json:"image,omitzero"` |
There was a problem hiding this comment.
There seems to be quite a few levels of nesting. Would we be able to just lift out the Ref and Profile to here directly?
From a higher level question, all the user is expected to do is set a list of UnmanagedCustomResourceDefinitions in the spec, right? What is defining the components in a normal workflow? Is this mostly for status? Or is some controller setting this for other controllers to consume?
There was a problem hiding this comment.
With the discriminated union it's hopefully a bit more obvious why this is nested.
Yeah, I'm not a huge fan of these things but they make @JoelSpeed happy 😜
I suspect that we will probably never add another member to the union, but given that the first draft had 2 it's far from impossible.
JoelSpeed
force-pushed
the
ccapio-staged
branch
from
e01cd64 to
2d7e886
Compare
JoelSpeed
force-pushed
the
ccapio-staged
branch
from
2d7e886 to
dfb24f9
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In `@operator/v1alpha1/types_clusterapi.go`:
- Line 54: Remove the stale TODO comment that questions the minimum number of
dots in CRD names from types_clusterapi.go; the validation and preceding comment
already state "CRD names must contain at least one '.' character" and the check
self.contains('.') enforces it, so delete the line containing "// TODO(Joel):
Check what the minimum number of dots is." to avoid confusion.
🧹 Nitpick comments (4)
payload-manifests/crds/0000_30_cluster-api_01_clusterapis-TechPreviewNoUpgrade.crd.yaml (1)
164-166: Port range validation in OCI image reference regex may reject valid ports.The regex
:[0-9]{2,5}for the port component requires 2-5 digits, which:
- Rejects valid single-digit ports (e.g., port 1-9)
- Accepts invalid ports > 65535 (e.g., 99999)
This is a minor edge case since single-digit ports are rarely used for registries, but consider adjusting to
:[0-9]{1,5}with an additional CEL rule for range validation if strict port validation is needed.operator/v1alpha1/tests/clusterapis.operator.openshift.io/ClusterAPIMachineManagement.yaml (2)
297-301: Inconsistent YAML formatting in test case.The list item formatting here differs from the rest of the file. Other component entries use:
- type: Image image: ref: ...But this entry has the
-on its own line with subsequent properties. While valid YAML, it's inconsistent with the pattern used elsewhere in this file (e.g., lines 186-190).📝 Suggested fix for consistency
- name: rev-2 revision: 2 contentID: content-2 components: - - - type: Image + - type: Image image: ref: quay.io/openshift/cluster-api@sha256:bbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbb profile: default
5-651: Consider adding a test for singleton constraint violation.The CRD enforces that
metadata.namemust be'cluster', but there's no test verifying that creating a ClusterAPI with a different name (e.g.,metadata.name: not-cluster) is rejected with the expected error message.📝 Suggested test case to add in onCreate section
- name: Should reject ClusterAPI with name other than 'cluster' initial: | apiVersion: operator.openshift.io/v1alpha1 kind: ClusterAPI metadata: name: not-cluster spec: {} expectedError: "clusterapi is a singleton, .metadata.name must be 'cluster'"operator/v1alpha1/types_clusterapi.go (1)
117-118: Add type-level documentation comment forClusterAPIInstallerRevision.This type lacks a doc comment, causing the generated swagger documentation to be inconsistent with other types (missing the empty string key in
map_ClusterAPIInstallerRevision). All other ClusterAPI-related types have doc comments.📝 Suggested fix
+// ClusterAPIInstallerRevision defines a revision of the ClusterAPI installation +// containing the components to be installed and their configurations. // +structType=atomic type ClusterAPIInstallerRevision struct {
| // alphanumeric characters, '-' or '.', and must start and end with an | ||
| // alphanumeric character, with a maximum length of 253 characters. | ||
| // CRD names must contain at least one '.' character. | ||
| // TODO(Joel): Check what the minimum number of dots is. |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
cat -n operator/v1alpha1/types_clusterapi.go | head -100Repository: openshift/api
Length of output: 6112
🏁 Script executed:
# Search for related validation rules and context
rg "contains.*\." operator/v1alpha1/types_clusterapi.go -B5 -A5Repository: openshift/api
Length of output: 907
🏁 Script executed:
# Search for CRD naming or validation patterns in the codebase
rg "minimum.*dot" --type go -iRepository: openshift/api
Length of output: 152
🏁 Script executed:
# Check for other TODOs related to naming validation
rg "TODO.*dot|TODO.*name" --type go -i -A2 -B2Repository: openshift/api
Length of output: 50369
🌐 Web query:
Kubernetes CRD CustomResourceDefinition naming format requirements specifications
💡 Result:
Kubernetes CRD naming / format requirements (CustomResourceDefinition)
1) CRD object name (metadata.name)
- Must be:
<spec.names.plural>.<spec.group>(for example:shirts.stable.example.com). [1][2]
2) API group (spec.group)
- Used in the API path:
/apis/<group>/...and must matchmetadata.nameas above. [2] - In practice, groups are expected to be DNS subdomain (RFC 1123) style (lowercase alphanumerics plus
-and., start/end alphanumeric, max 253 chars). [3]
3) Resource & Kind names (spec.names.*)
spec.names.kind: typically singular CamelCase (for exampleShirt). [2]spec.names.plural: all lowercase, and is the REST resource name in the URL (for example/.../shirts). Must matchmetadata.namevia<plural>.<group>. [2]spec.names.singular: all lowercase (defaults to the lowercased kind). [2]spec.names.shortNames[]: if set, each must be all lowercase. [2]
4) Custom resource instance names (metadata.name on your CRs)
- Follow Kubernetes object naming constraints; commonly DNS subdomain (RFC 1123) rules (same character/length rules as above). [3]
Sources:
[1] https://pkg.go.dev/k8s.io/apiextensions-apiserver/pkg/apis/apiextensions#CustomResourceDefinition
[2] https://kubernetes.io/zh-cn/docs/reference/kubernetes-api/extend-resources/custom-resource-definition-v1/
[3] https://kubernetes.io/docs/concepts/overview/working-with-objects/names/
Remove the TODO comment—the minimum dot requirement is already documented and validated.
The comment on line 53 already explicitly states that "CRD names must contain at least one '.' character." This is correct per Kubernetes CRD naming conventions (format: <plural>.<group>), and the validation rule on line 67 (self.contains('.')) enforces it. The TODO should be removed since the uncertainty has been resolved and is documented directly above it.
🤖 Prompt for AI Agents
In `@operator/v1alpha1/types_clusterapi.go` at line 54, Remove the stale TODO
comment that questions the minimum number of dots in CRD names from
types_clusterapi.go; the validation and preceding comment already state "CRD
names must contain at least one '.' character" and the check self.contains('.')
enforces it, so delete the line containing "// TODO(Joel): Check what the
minimum number of dots is." to avoid confusion.
dfb24f9 to
2a4d5ac
Compare
|
Reviewed and updated this synchronously with @mdbooth /lgtm |
openshift-ci-robot
added
the
verified
|
@JoelSpeed: This PR has been marked as verified by DetailsIn response to this:
Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the openshift-eng/jira-lifecycle-plugin repository. |
|
Scheduling tests matching the |
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: JoelSpeed The full list of commands accepted by this bot can be found here. The pull request process is described here DetailsNeeds approval from an approver in each of these files:
Approvers can indicate their approval by writing |
openshift-ci
bot
added
the
approved
|
/test e2e-aws-serial-1of2 |
|
PR-Agent: could not fine a component named |
|
/test integration |
|
PR-Agent: could not fine a component named |
|
@mdbooth: all tests passed! Full PR test history. Your PR dashboard. DetailsInstructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes-sigs/prow repository. I understand the commands that are listed here. |
6 participants
No description provided.